-
Notifications
You must be signed in to change notification settings - Fork 501
Report file names in which "Unexpected Julia interpolation" occurred #2793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report file names in which "Unexpected Julia interpolation" occurred #2793
Conversation
|
In my testing, when I introduce a deliberate error in Oscar's |
could probably be better rephrased to
|
640dffe to
34bfb79
Compare
…tion problems in markdown
Rephrase to be not quite so informal Co-authored-by: Max Horn <[email protected]>
34bfb79 to
827e554
Compare
src/html/HTMLWriter.jl
Outdated
| This is on the page $(locrepr(dctx.navnode.page)), with parent $(dctx.navnode.parent), and we | ||
| were given the value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaruni96 PR #2803 is now merged. Can you please resolve the conflicts in CHANGELOG.md and then try this:
| This is on the page $(locrepr(dctx.navnode.page)), with parent $(dctx.navnode.parent), and we | |
| were given the value: | |
| This is in file $(locrepr(dctx)), and we were given the value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The missing docs before src is to be expected, given that Oscar's docs/make.jl does this:
cd(joinpath(oscardir, "docs")) do
...
makedocs(;
...
end
and the file names/paths we print are relative to the current working directory.
I am not sure why Oscar does that, it would be nice if we could get rid of it, but that's for another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then try this:
This doesn't work, complains about wrong data types. I tried monkey patching a few things hoping its a trivial error, but I couldn't make any headway (just more and different errors).
The error I get is
ERROR: FieldError: type Documenter.HTMLWriter.HTMLContext has no field `page`, available fields: `doc`, `settings`, `scripts`, `documenter_js`, `themeswap_js`, `warner_js`, `search_index`, `search_index_js`, `search_navnode`, `atexample_warnings`
Stacktrace:
[1] getproperty
@ ./Base_compiler.jl:54 [inlined]
[2] locrepr(dctx::Documenter.HTMLWriter.DCtx, lines::Nothing)
@ Documenter.HTMLWriter ~/Programs/git/GitHub/Documenter.jl/src/html/HTMLWriter.jl:715
[3] locrepr(dctx::Documenter.HTMLWriter.DCtx)
@ Documenter.HTMLWriter ~/Programs/git/GitHub/Documenter.jl/src/html/HTMLWriter.jl:713
[4] domify(dctx::Documenter.HTMLWriter.DCtx, ::MarkdownAST.Node{Nothing}, e::MarkdownAST.JuliaValue)
@ Documenter.HTMLWriter ~/Programs/git/GitHub/Documenter.jl/src/html/HTMLWriter.jl:2446
[5] domify(dctx::Documenter.HTMLWriter.DCtx, node::MarkdownAST.Node{Nothing})
@ Documenter.HTMLWriter ~/Programs/git/GitHub/Documenter.jl/src/html/HTMLWriter.jl:1813
[6] #domify##2
@ ~/Programs/git/GitHub/Documenter.jl/src/html/HTMLWriter.jl:1816 [inlined]
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh. Seems my previous PR did not have enough coverage to reveal that I messed up... Try applying this patch in addition:
diff --git a/src/html/HTMLWriter.jl b/src/html/HTMLWriter.jl
index 6868a874c..2803165fd 100644
--- a/src/html/HTMLWriter.jl
+++ b/src/html/HTMLWriter.jl
@@ -710,9 +710,7 @@ struct DCtx
end
function Documenter.locrepr(dctx::DCtx, lines = nothing)
- doc = dctx.ctx.doc
- page = dctx.navnode.page
- return Documenter.locrepr(dctx.ctx.doc, dctx.ctx.page, lines)
+ return Documenter.locrepr(dctx.ctx.doc, dctx.navnode.page, lines)
end
function SearchRecord(ctx::HTMLContext, navnode; fragment = "", title = nothing, category = "page", text = "")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without trying it now, relying on my memory of attempting something like this during my "monkey patching", I expect this to not work because dctx.navnode.page is a string, but locrepr expects something of type ::Page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, my patch is patching the locrepr method I added earlier today. It had a merge conflict which I resolved incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After applying your patch, I still get the error I predicted :
ERROR: FieldError: type String has no field `source`; String has no fields at all.
Stacktrace:
[1] getproperty
@ ./Base_compiler.jl:54 [inlined]
[2] locrepr(doc::Documenter.Document, page::String, lines::Nothing)
@ Documenter ~/Programs/git/GitHub/Documenter.jl/src/utilities/utilities.jl:84
[3] locrepr(dctx::Documenter.HTMLWriter.DCtx, lines::Nothing)
@ Documenter.HTMLWriter ~/Programs/git/GitHub/Documenter.jl/src/html/HTMLWriter.jl:713
The locrepr method in utilities.jl doesn't specify needing the second argument as a ::Page, but assumes it, and tries accessing the field source.
|
I've applied the adjustments and fixed the errors. At least in local tests it works fine (it's a bit hard to tell exactly without #2804 merged, and without the dedicated tests this really deserves, but we'll get there) |
|
In my testing, I get With your earlier comment,
I think, this is ready to be merged. |
|
Taking a step back and thinking about this again, I wonder: perhaps we should simply perform this check earlier. Namely when we ingest docstrings (at which point we have precise location information), we could run a quick check on what we got and look for this issue, and report it then. This way, not only do we get better location info, but also we don't have to implement this for every writer again. Thoughts @mortenpi ? (To be clear, I'd still want to merge this PR, as it is an improvement now) |
Now is more in line with our other warnings
fingolfin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I resolved conflicts, adjusted to the new "warning tests", then used the opportunity to in fact further adjust those warning messages to be properly indented.
Fixes #2752
Depends on #2792